Conversation
When --channel-name is provided without a direct recipient, the push notification is published to the channel with the payload wrapped in extras.push, routing it to push-subscribed devices via the channel. If a direct recipient (--device-id, --client-id, --recipient) is also present, --channel-name is ignored with a warning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe changes add support for publishing push notifications via a channel in addition to device/client recipients. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Command
participant Validation
participant Publisher
participant PushAdmin as Push Admin API
participant Channels as Channels API
participant Output
User->>CLI: ably push publish --channel-name=my-channel
CLI->>Validation: Validate flags<br/>(has channel-name OR recipient?)
Validation-->>CLI: ✓ Valid
CLI->>Publisher: Route to publisher<br/>(hasDirectRecipient?)
alt Channel-based Publishing
Publisher->>Channels: Get channel instance
Channels->>Channels: publish({<br/>extras: { push: payload }<br/>})
Channels-->>Publisher: ✓ Published
Publisher->>Output: Format response<br/>({ published: true, channel })
else Recipient-based Publishing (if recipient provided)
Publisher->>PushAdmin: publish(recipient, payload)
PushAdmin-->>Publisher: ✓ Published
Publisher->>Output: Format response<br/>(success message)
end
Output-->>User: Display result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
maratal
changed the title
channel-name flag to push publish
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/push/publish.ts`:
- Line 236: The chained call rest.channels.get(channelName).publish({ extras: {
push: payload } }) is too long for Prettier; split the expression across
multiple lines to satisfy line-length rules. Specifically, break after getting
the channel (rest.channels.get(channelName)) and place the .publish( call on the
next line, with the object literal formatted over multiple lines (e.g., open
.publish( on its own line, then place extras and push on separate indented
lines) and close the call on a new line. Keep the same identifiers (rest,
channels, get, channelName, publish, payload) and behavior unchanged.
- Around line 99-105: The warning emitted when hasDirectRecipient and
flags["channel-name"] is true uses this.log(formatWarning(...)) and will corrupt
--json output; wrap that this.log call in a guard that checks
this.shouldOutputJson(flags) (i.e., only call this.log when
!this.shouldOutputJson(flags)) so the formatWarning message is emitted only for
human-readable runs; update the block around the this.log/formatWarning
invocation in publish (where hasDirectRecipient and flags["channel-name"] are
checked) to use the conditional before logging.
In `@test/unit/commands/push/publish.test.ts`:
- Around line 148-157: The test currently JSON.parses stdout directly which can
fail due to Prettier/whitespace formatting; update the test to trim the CLI
output before parsing by calling JSON.parse(stdout.trim()) so it tolerates
trailing newlines/formatting. Locate the test using runCommand in
publish.test.ts (the it block "should output JSON with channel when publishing
via channel") and change JSON.parse(stdout) to JSON.parse(stdout.trim()) to make
the assertion robust.
- Around line 173-183: Formatting in the "should handle channel publish errors"
test is inconsistent with project Prettier rules; reformat the test in
test/unit/commands/push/publish.test.ts (the it(...) block) so it matches the
code style: proper indentation, spacing around arrow functions, consistent
semicolons, and no stray blank/merged lines — specifically fix the block that
uses getMockAblyRest(),
mock.channels._getChannel("err-channel").publish.mockRejectedValue(new
Error("Channel error")), and the await runCommand([...], import.meta.url) call
so Prettier passes; you can run Prettier or your project's formatter to apply
the exact corrections.
- Around line 105-134: The tests "should publish via channel wrapping payload in
extras.push" and "should ignore --channel-name when --device-id is also
provided" have long single-line argument arrays and matcher chains that fail
Prettier; reformat the runCommand calls and expect(...) assertions into
multiline style so each argument or chained matcher is on its own line (e.g.,
break the runCommand argument array across multiple lines and break the
expect.objectContaining/expect.anything matcher chains into multiple indented
lines), keeping the same values and assertions for getMockAblyRest(),
runCommand(...), channel.publish expectations, and mock.push.admin.publish
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e84d3f9a-95cf-4a66-bd46-ad5042445fcd
📒 Files selected for processing (3)
README.mdsrc/commands/push/publish.tstest/unit/commands/push/publish.test.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
src/commands/push/publish.ts
Outdated
| description: "Raw recipient JSON for advanced targeting", | ||
| exclusive: ["device-id", "client-id"], | ||
| }), | ||
| "channel-name": Flags.string({ |
There was a problem hiding this comment.
On other commands we use --channel so we should have the same here
There was a problem hiding this comment.
Done — renamed to --channel.
There was a problem hiding this comment.
You should update PR summary about using --channel-name to --channel
There was a problem hiding this comment.
Updated the PR description to reflect --channel throughout.
src/commands/push/publish.ts
Outdated
| } | ||
|
|
||
| if (hasDirectRecipient && flags["channel-name"]) { | ||
| this.log( |
There was a problem hiding this comment.
Per coderabbit, this would break json mode
…-json - Rename flag to --channel for consistency with other commands (per Andy) - Wrap --channel-ignored warning in !shouldOutputJson() guard to avoid polluting machine-readable output (per CodeRabbit/Andy) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
src/commands/push/publish.ts
Outdated
|
|
||
| if (hasDirectRecipient && flags.channel && !this.shouldOutputJson(flags)) { | ||
| this.log( | ||
| formatWarning( |
There was a problem hiding this comment.
I feel this warning should also be available in JSON mode.
You can check ephemeralSpaceWarning at https://github.com/ably/ably-cli/pull/185/changes
There was a problem hiding this comment.
Fixed — now emits logJsonStatus("warning", ...) when --json is active, following the same pattern as ephemeralSpaceWarning in #185.
src/commands/push/publish.ts
Outdated
| if (!hasDirectRecipient && !flags.channel) { | ||
| this.fail( | ||
| "A recipient is required: --device-id, --client-id, or --recipient", | ||
| "A target is required: --device-id, --client-id, --recipient, or --channel-name", |
There was a problem hiding this comment.
| "A target is required: --device-id, --client-id, --recipient, or --channel-name", | |
| "A target is required: --device-id, --client-id, --recipient, or --channel", |
maratal
changed the title
channel-name flag to push publishchannel flag to push publish
…emit channel-ignored warning in JSON mode Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Manual testing —
|
| } | ||
| } else { | ||
| this.log(formatSuccess("Push notification published.")); | ||
| const channelName = flags.channel!; |
There was a problem hiding this comment.
Btw I feel, if possible we should also introduce confirmation dialogue saying
Push notification will be sent to all users subscribed to this channel or channels, do you want to publish to all users present on the channel
using promptForConfirmation in the code.
Generally devs tend to test push notifications, if there are lots of users present on the channel in prod environment, all of them will receive test notifications. So, we can make it explicit in the prompt.
There was a problem hiding this comment.
Same, goes for batch-publish on given set of channels
There was a problem hiding this comment.
Good call. Added promptForConfirmation before the channel publish in push publish and batch-publish — it warns that the notification will go to all devices subscribed to the channel, and is skipped in --json mode or with the new --force/-f flag for scripted use.
…mpts - batch-publish now accepts items with a "channel" field as an alternative to "recipient"; channel items are published via extras.push on the channel (the /push/batch/publish API only accepts recipient-based items) - If both "channel" and "recipient" are present in a batch item, "channel" is ignored with a warning (consistent with push publish behaviour) - Added promptForConfirmation before publishing to channels in both push publish and push batch-publish, as channel publishes reach all subscribed devices — skipped in --json mode and with new --force/-f flag - Added --force flag to push publish and push batch-publish to skip the confirmation prompt in scripts/CI Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| $ ably push config show | ||
|
|
||
| COMMANDS | ||
| ably push batch-publish Publish push notifications to multiple recipients in a batch |
There was a problem hiding this comment.
Add more examples for batch-publish both in code and READEME ( README will be automatcally updated once examples are added in the code )
There was a problem hiding this comment.
Also, verify that the examples for both publish and batch-publish align correctly with the specified parameters. Additionally, review all examples under ably push, as these will be the primary references for developers using the cli.
|
|
||
| static override examples = [ | ||
| '<%= config.bin %> <%= command.id %> --payload \'[{"recipient":{"deviceId":"dev1"},"payload":{"notification":{"title":"Hello","body":"World"}}}]\'', | ||
| '<%= config.bin %> <%= command.id %> --payload \'[{"channel":"my-channel","payload":{"notification":{"title":"Hello","body":"World"}}}]\'', |
There was a problem hiding this comment.
Hmm.. not sure what format does those examples follow, can we make them similar to examples format in other commands. e.g. ably push
|
|
||
| static override examples = [ | ||
| '<%= config.bin %> <%= command.id %> --payload \'[{"recipient":{"deviceId":"dev1"},"payload":{"notification":{"title":"Hello","body":"World"}}}]\'', | ||
| '<%= config.bin %> <%= command.id %> --payload \'[{"channel":"my-channel","payload":{"notification":{"title":"Hello","body":"World"}}}]\'', |
There was a problem hiding this comment.
Can we make command to accept space separated channel names, so internally it gets accepted as channels string array. If payload is same for all channels, then this would make sense right?
You can check channels subscribe command for the same 👍
There was a problem hiding this comment.
Pull request overview
Adds channel-based push publishing support so push notifications can be routed via Ably channels (wrapping payload under extras.push), with safeguards/warnings when mixed targeting flags are used.
Changes:
- Add
--channel(and--forceconfirmation bypass) topush publish, publishing viachannel.publish({ extras: { push: ... } }). - Extend
push batch-publishto support batch items targetingchannel, including confirmation prompting and warnings when bothrecipientandchannelare present. - Update unit/e2e tests and README text to reflect channel targeting.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/commands/push/publish.ts |
Adds --channel targeting path (via extras.push), warning behavior, and confirmation prompting (--force). |
src/commands/push/batch-publish.ts |
Splits batch items into recipient vs channel publishing paths; adds confirmation + warning behavior and --force. |
test/unit/commands/push/publish.test.ts |
Adds unit coverage for channel publishing, warning behavior, JSON output, and channel publish errors. |
test/unit/commands/push/batch-publish.test.ts |
Adds unit coverage for channel items in batch payloads, confirmation bypass, and validation. |
test/e2e/push/publish-e2e.test.ts |
Updates expected validation error message to “A target is required”. |
README.md |
Updates push publish wording/usage/examples for channel targeting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -3499,13 +3502,15 @@ | |||
| --web=<value> Web push-specific override as JSON | |||
There was a problem hiding this comment.
push publish now includes a --force flag (and channel publishing prompts for confirmation when not using JSON), but the README section for ably push publish doesn’t list --force in USAGE/FLAGS. Please update the generated command docs so help/README stay in sync with the command’s actual flags and behavior.
| for (const [i, result] of responseItems.entries()) { | ||
| if ( | ||
| result.error || | ||
| (result.statusCode && result.statusCode !== 200) | ||
| ) { | ||
| const err = result.error as Record<string, unknown> | undefined; | ||
| failedItems.push({ | ||
| index: i, | ||
| error: String(err?.message ?? "Unknown error"), | ||
| }); | ||
| } else { | ||
| succeeded++; | ||
| } | ||
| } | ||
| if (responseItems.length === 0) { | ||
| succeeded += recipientItems.length; | ||
| } | ||
| } | ||
|
|
||
| // Publish channel-based items via channel extras.push | ||
| for (const [i, item] of channelItemsList.entries()) { | ||
| try { | ||
| await rest.channels | ||
| .get(item.channel) | ||
| .publish({ extras: { push: item.payload } }); | ||
| succeeded++; | ||
| } catch (error) { | ||
| failedItems.push({ | ||
| index: recipientItems.length + i, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); |
There was a problem hiding this comment.
In mixed recipient/channel batches, the failedItems.index values don’t correspond to the original batchPayload indices: recipient failures use the index within responseItems (subset order), and channel failures use recipientItems.length + i (also subset-based). This makes it hard/impossible for users to map failures back to their input array. Consider storing the original input index alongside each queued item and using that original index for both success/failure reporting.
| for (const [i, item] of channelItemsList.entries()) { | ||
| try { | ||
| await rest.channels | ||
| .get(item.channel) | ||
| .publish({ extras: { push: item.payload } }); | ||
| succeeded++; | ||
| } catch (error) { | ||
| failedItems.push({ | ||
| index: recipientItems.length + i, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); |
There was a problem hiding this comment.
Channel-targeted items are published one-by-one with an await inside the loop. With the current max batch size (10,000), this can turn into thousands of sequential network requests and be extremely slow / prone to rate limits. Consider grouping items by channel and publishing arrays per channel (or running publishes with a concurrency limit) to reduce request count and wall-clock time.
| for (const [i, item] of channelItemsList.entries()) { | |
| try { | |
| await rest.channels | |
| .get(item.channel) | |
| .publish({ extras: { push: item.payload } }); | |
| succeeded++; | |
| } catch (error) { | |
| failedItems.push({ | |
| index: recipientItems.length + i, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| if (channelItemsList.length > 0) { | |
| // Group items by channel so we can publish multiple messages per channel in one request | |
| const channelGroups = new Map< | |
| string, | |
| { messages: { extras: { push: unknown } }[]; indices: number[] } | |
| >(); | |
| channelItemsList.forEach((item, i) => { | |
| const globalIndex = recipientItems.length + i; | |
| const message = { extras: { push: item.payload as unknown } }; | |
| const existing = channelGroups.get(item.channel); | |
| if (existing) { | |
| existing.messages.push(message); | |
| existing.indices.push(globalIndex); | |
| } else { | |
| channelGroups.set(item.channel, { | |
| messages: [message], | |
| indices: [globalIndex], | |
| }); | |
| } | |
| }); | |
| for (const [channel, group] of channelGroups.entries()) { | |
| try { | |
| await rest.channels.get(channel).publish(group.messages); | |
| succeeded += group.messages.length; | |
| } catch (error) { | |
| const errorMessage = | |
| error instanceof Error ? error.message : String(error); | |
| for (const index of group.indices) { | |
| failedItems.push({ | |
| index, | |
| error: errorMessage, | |
| }); | |
| } | |
| } |
| channelItemsList.push({ | ||
| channel: entry.channel as string, |
There was a problem hiding this comment.
entry.channel is cast to string without validating its type/value. Since the batch payload is user-provided JSON, channel could be a non-string (object/number/null), which would produce confusing behavior (e.g., "[object Object]") or downstream SDK errors. Consider explicitly validating typeof entry.channel === "string" and that it’s non-empty when entry.recipient is not provided.
| channelItemsList.push({ | |
| channel: entry.channel as string, | |
| const channel = entry.channel; | |
| if (typeof channel !== "string" || channel.trim().length === 0) { | |
| this.fail( | |
| `Item at index ${index} has an invalid "channel" field; expected a non-empty string when "recipient" is not provided`, | |
| flags as BaseFlags, | |
| "pushBatchPublish", | |
| ); | |
| } | |
| channelItemsList.push({ | |
| channel, |
| if (this.shouldOutputJson(flags)) { | ||
| this.logJsonResult( | ||
| { | ||
| published: true, | ||
| total: batchPayload.length, | ||
| total, | ||
| succeeded, | ||
| failed: failed.length, | ||
| ...(failed.length > 0 ? { failedItems: failed } : {}), | ||
| failed: failedCount, | ||
| ...(failedCount > 0 ? { failedItems } : {}), | ||
| }, |
There was a problem hiding this comment.
The JSON output schema for failures changes here: previously failedItems contained the raw response items from /push/batch/publish, but it’s now an array of { index, error }. If consumers rely on the old structure (e.g., statusCode, error.code), this is a breaking change. Consider preserving the prior structure (and adding index), or documenting/versioning the schema change.
| @@ -28,6 +32,10 @@ export default class PushBatchPublish extends AblyBaseCommand { | |||
| description: "Batch payload as JSON array, @filepath, or - for stdin", | |||
| required: true, | |||
| }), | |||
| force: Flags.boolean({ | |||
| char: "f", | |||
| description: "Skip confirmation prompt when publishing to channels", | |||
| }), | |||
There was a problem hiding this comment.
This PR introduces channel-targeted batch publishing (payload items with a channel field) and a new --force flag for push:batch-publish, but the PR description/release notes only mention changes to push publish. Consider updating the PR description and/or release notes to reflect the additional user-facing behavior added in this command too.
Summary
--channelflag topush publishcommandextras.push, routing it to push-subscribed devices via the channel--device-id,--client-id,--recipient) is also provided,--channelis silently ignored with a warning (emitted in both human-readable and JSON modes)Summary by CodeRabbit
Release Notes
New Features
--channelflag toably push publishfor publishing push notifications to channels. When both recipient flags (e.g.,--device-id) and--channelare provided, the channel flag is ignored with a warning.Documentation
ably push publishcommand documentation to expand targeting scope from "device or client" to "device, client, or channel".ably channels presence updatewith--pretty-jsonformatting.